This timeout is now increased dynamically with each failed connection attempt.

Hmm, fair point that the timeout could be increased at the client level. That said, some gRPCs inherently take much longer than others (e.g., list_gridpool_orders), which is why I was thinking of having this handled at the gRPC level. However, I’m definitely not opposed to managing it at the client level either.

For me, the most important thing is that we have a timeout in place (whether it’s dynamically decided or fixed) to avoid hanging calls.

I see. Different RPCs having different timeouts needs to be considered, true.

The main objective of timeouts would be to reduce stress on the client's connection. Having per-call timeout violates this principle. One call might be backing off, but then a new call might go straight through. So my guess is that while you may see some benefits of this approach right away, it would be a matter of chance when the connection to the server gets stressed. The more client apps, the further it will regress.

I currently have a max_timeout (maximum timeout duration for gRPC calls), timeout_increment (increment value for timeout on each retry, e.g. 20secs) and a max_timeout_retries (maximum number of retry attempts when a timeout is reached). Having those 3 vars could ba a bit of an overkill, I could remove one of them if needed.

Sorry, I'm just realizing this has become a retry implementation for non-streaming calls.

Retries of this sort have to be infinite because we have to keep trying until the API call succeeds, and doing that in the client would take away the actor's agency in deciding what to do when there is a client error, like taking fallback action, sending a slack message, etc.

It also introduces the risk of creating orders with outdated power or price forecasts if the client's retry succeeds after a few minutes.

For this reason, all of our clients should have retries only for streaming APIs (which are taken care of by the GrpcStreamBroadcaster in the base client).

For non-streaming APIs, retries can be implemented in the actors using the ExponentialBackoff provided by the base client. I'm also adding a receiver interface for this in the base client, allowing us to use a select loop to decide when to retry.

Also, I've implemented these in the actor for now in this PR: https://github.com/frequenz-io/frequenz-actor-electricity-trading/pull/249

So, I think this PR should go back to just implementing a timeout for non-streaming methods to prevent them from blocking for too long and limiting the actor's ability to take fallback action.

Then all that needs to be done is invoke only simple RPCs with the backoff, and call the streaming ones directly. If a backoff module exists in the base client, then that could be used directly (and sorry that you implemented it here @camille-bouvy-frequenz).

I also would not recommend going back to a non-global backoff, since I do not see a value in that.

Then all that needs to be done is invoke only simple RPCs with the backoff

Not in the client, for the reasons mentioned above. We just need timeout to be implemented in this PR.

I also would not recommend going back to a non-global backoff, since I do not see a value in that.

Top level retry is provided by the actor implementation. That gives the most flexibility. And there are plans to add additional backoff mechanisms and failure detection based on run times, etc. And then it is needed only in one place. If users need additional control, they can do it in their actors.

Then all that needs to be done is invoke only simple RPCs with the backoff

Not in the client, for the reasons mentioned above. We just need timeout to be implemented in this PR.

I also would not recommend going back to a non-global backoff, since I do not see a value in that.

Top level retry is provided by the actor implementation. That gives the most flexibility. And there are plans to add additional backoff mechanisms and failure detection based on run times, etc. And then it is needed only in one place. If users need additional control, they can do it in their actors.

So if I understand it right, you specifically are suggesting having per-RPC backoffs in this client? If yes, how would that help?

Nice that a global backoff module exists in the actor library!

you specifically are suggesting having per-RPC backoffs in this client?

No, I'm saying we shouldn't add automatic retry in the client for non-streaming RPCs. There is no question of backoff if there's no retry.

I'm only suggesting that we have timeout for such RPCs.